-
-
Notifications
You must be signed in to change notification settings - Fork 13.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
python3Packages.plover-lapwing-aio: init at 1.3.4 #347354
Conversation
I didn't understand. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't add an application to python-modules
. If you want to add a plugin system, take a look at poetry
and nbqa
.
We also recommend that you read the python packaging guide carefully and submit several small PRs, rather than putting a lot of changes into a single PR. This makes it easier for us to review and fix.
Thanks.
My approach here is very similar to
Could you be more specific? I think this adheres to most of those points. I'll add
I didn't think this was a lot of changes — it's just a few small packages and they're mostly interdependent 😅 I can break it down into some smaller PRs but they'll need to be merged in a specific order. |
6311882
to
754cd13
Compare
Please stop sending a number of pings. |
Oh I was just breaking this down into smaller pull requests as requested. I'll close this for a moment while I fiddle with it to avoid notifying you. |
plover
: move into pythonPackages
and add plugins
Just because a plugin requires it does not mean it is a library. It is still an application and should be outside python-modules.
As is often the case with python applications, we can remove
I would like you to check the list I have shown you one by one, but some packages have no pyprojects, build-systems and tests, and the commit messages do not match our guideline.
It is a bit much to change, but I don't think it is too much if you are familiar with this area and follow the above list well. I am not asking you to split up your PRs and send them all at once. I am asking you to submit a small PR, review it, revise it, merge it, and repeat the cycle. |
That seems sensible. I'll move it back into
I suppose that we can, but isn't it better to declare the real dependencies at build time? If you think we should do it this way I'm happy to try.
Unfortunately I think it is quite difficult to accurately review a small leaf of this dependency tree without reference to the packages that will use it. Certainly the resulting PRs cannot be tested in any meaningful way. Of course we can definitely work our way from one package to the next if that's more convenient for your workflow. I would have imagined this is more work for you, especially without being able to see the whole picture, but I'm happy to do whatever you prefer. |
This is the last PR in a series of PRs that:
plover
from4.0.0.dev10
to the more recent4.0.0rc2
pythonPackages
plover
fromapplications/misc
todevelopment/python-modules
to allow using it as a dependency for the pluginswithPlugins
function toplover
plover-lapwing-aio
plugin and its dependenciesThings done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.